ADFA-4739: Pipeline for producing template-based Kotlin documentation - #21

Blocking — unrecoverable data loss against the production DB.

A single dead TooltipButtons.uri takes out the entire parent Tooltips record — the hand-authored summary/detail plus every other, still-valid button hanging off it.

Failure path: a kotlin-stdlib symbol gets renamed upstream, so k/kotlin-stdlib/kotlin.text/old-name/index.html has no matching JSON and its Content row is deleted. cleanup_orphaned_tooltips then sees one orphaned button URI and deletes the whole tooltip. That tooltip text is not in the plugin output and cannot be regenerated — it is only recoverable from the .bak. file.

Two things make this sharp rather than theoretical:

• build-kotlin-docs.yaml runs step 5/5 unconditionally (no skip_website_docs guard) against the real documentation.db.
• dry_run gates only the Drive upload, not this mutation — so a "dry run" still destroys tooltips in the downloaded DB, and a subsequent non-dry run publishes that.

Suggested fix: only delete a Tooltips row when all of its buttons are orphaned, and drop individual dead TooltipButtons rows otherwise. Even then, please log every tooltip id being deleted, and consider requiring an explicit --prune-tooltips flag so the destructive path is opt-in rather than a side effect of a docs rebuild.

Blocking — silent failure: broken DB publishes with exit 0.

A page that fails to convert is logged to stderr and skipped, but the process still exits 0. (Same pattern at md_to_json.py:607.)

If 40 topics hit an unhandled markdown-it edge case, this prints Converted 960/1000 pages to stderr and exits clean. The workflow then sails through the blacklist verification — which only checks that blacklisted pages are absent, not that expected pages are present — uploads the artifact, and with dry_run: false overwrites the production database with 40 pages missing and 40 dangling nav/prev-next links pointing at them.

Nothing anywhere in the pipeline fails on a non-zero skip count.

Suggested fix: count the skips and exit non-zero at the end if any occurred, with an explicit --allow-conversion-failures N escape hatch if you want tolerance for a known-bad page:

failed = []
# ... in the except: failed.append((md_path, exc))
if failed:
    print(f"{len(failed)} page(s) failed to convert:", file=sys.stderr)
    for p, e in failed:
        print(f"  {p}: {e}", file=sys.stderr)
    if len(failed) > args.allow_conversion_failures:
        sys.exit(1)

Blocking — this silently destroys every raw HTML table in the docs.

There's no word boundary after the tag alternation, so <table> / </table> match as tab container markers.

Reproduced by running the converter:

input:  <table>\n<tr><td>Kotlin 2.0</td><td>Supported</td></tr>\n</table>
output: [{"type":"tab","attrs":{},"blocks":[{"type":"html","html":"<tr><td>Kotlin 2.0</td>..."}]}]

The <table>/</table> lines are swallowed, and page.peb's b.type == "tab" branch then emits bare <tr>/<td> with no table wrapper — which browsers discard outright. <table class="x"> matches too, yielding garbage attrs parsed from le class="x".

This module's own docstring notes that pages like roadmap.md are "basically hand-written HTML tables", so this is hitting real content. And the comment at templates/page.peb:128-132 ("A lone <tab> not wrapped in <tabs> (e.g. seen in eap.json's HTML-table compatibility layout)") is this bug's symptom being papered over in the template rather than fixed here.

Suggested fix — add \b, and while you're here make the self-closing case explicit (see my comment on line 396):

TAG_RE = re.compile(r"^<(/?)(tabs|tab|note|tip|warning)\b([^>]*?)(/?)>$", re.I)

With \b, <table> no longer matches (tab can't be followed by l), while <tab>, <tab id="x"> and <tabs> all still do. Worth adding a regression test for <table> and <tabs> side by side.

Blocking — both Drive downloads will 403/404 on the first real run.

drive.file only grants access to files the service account itself created. It cannot read a pre-existing Drive file, which is exactly what check-tools/download_database.py does here (service.files().get(fileId=...) / get_media).

Compare docdb-regression-test.yaml:46-48, which downloads the same GOOGLE_DRIVE_FILE_ID via the same script and requests both scopes. This workflow dropped drive.readonly, so "Download current documentation.db" and "Download Writerside image export" will both fail.

A filename collision upstream takes down the whole run.

Page ids are flattened to the bare .md stem with no collision handling, and Content.path has an enforced UNIQUE constraint (verified: sqlite_autoindex_Content_1).

If kotlin-web-site ever adds e.g. topics/tour/overview.md alongside topics/native/overview.md, both produce db_id = "k/html/overview". The second INSERT at line 550 raises sqlite3.IntegrityError: UNIQUE constraint failed: Content.path, the transaction rolls back, and the CI job dies with a traceback.

The docstring asserts that "filenames are already unique across the whole topics/ tree" — but that's a third-party repo that changes weekly, so it's an assumption with an expiry date rather than an invariant. Note that the neighbouring code already takes this more seriously: md_to_json.build_topic_index tolerates collisions via setdefault, and build_image_index explicitly warns on the analogous image case. This path does neither.

At minimum, detect the collision and fail with a clear message naming both files, rather than letting it surface as an opaque IntegrityError 70 lines later. Better still, derive the id from the path relative to topics/ so it can't collide.

Walking work_dir instead of this run's manifest lets stale files from a previous run into the DB.

work_dir is only auto-removed when it's the temp fallback (work_dir_is_temp). When passed as the third positional argument it's mkdir(exist_ok=True)'d and never cleaned — so a reused --work-dir accumulates.

Two concrete misbehaviours:

• Run once with --webp (produces mascot.webp), then re-run without --webp into the same dir: this rglob("*") now finds both mascot.png and the stale mascot.webp, and inserts both.
• A media file deleted from media_dir between runs still sits in work_dir and gets re-inserted — resurrecting content this run was supposed to drop.

The rename_map is built only from this run's manifest, so it knows nothing about either case.

Fix: iterate the manifest (which is already the authoritative record of what this run produced) rather than the directory listing. If you'd rather keep the rglob, clear work_dir at the start of every run.

Minor, but worth noting: sorted(...) over the whole tree also means the fix is cheap either way.

Emitting a second style attribute instead of merging — the coloring is silently dropped.

Reproduced:

input:  <p><a href="https://example.com" style="font-weight:bold">ext</a></p>
output: <a href="https://example.com" style="font-weight:bold" style="color: #cc0000;">

HTML parsers keep the first style and discard duplicates, so the broken/external-link indicator that config.json's broken-ext-link-color exists to provide never renders — and it fails specifically on hand-authored HTML passthrough links, which are the ones most likely to already carry an inline style.

Merge into the existing attribute if present (append ;color: ... to its value), and only append a fresh style= when there isn't one.

The inputs are documented as accepting a commit SHA, but git clone --branch doesn't take one.

Both input descriptions say "Branch/tag/commit of ...". Passing a SHA gives:

git clone --depth 1 --branch <sha> https://github.com/JetBrains/kotlin.git
fatal: Remote branch <sha> not found in upstream origin

Same issue on the kotlin_ref clone below — whose description explicitly frames pinning as the route to "a reproducible build", i.e. precisely the case that fails.

Either fix the docs to say "branch or tag", or support SHAs properly:

git init kotlin-web-site && cd kotlin-web-site
git remote add origin https://github.com/JetBrains/kotlin-web-site.git
git fetch --depth 1 origin "$KOTLIN_WEB_SITE_REF"
git checkout FETCH_HEAD

Given reproducibility is the stated goal, I'd lean toward supporting SHAs.

Unpinned bare pip install, and requirements.txt was never updated with the new dependencies.

Two separate problems here:

1. Missing deps. markdown-it-py, scour and cairosvg are absent from requirements.txt — they're only installed by the ad-hoc pip install on line 155. Confirmed locally: after a plain pip install -r requirements.txt, import md_to_json fails with ModuleNotFoundError: No module named 'markdown_it'. So anyone following README.md outside CI gets a broken checkout. These belong in requirements.txt.

2. No pinning, and inconsistent with the rest of the repo. Nothing is version-pinned and there's no lockfile, so an upstream markdown-it-py major bump breaks the build with nothing to roll back to. The repo's other workflows already use uv — docdb-regression-test.yaml uses uv sync/uv run, publish-doc-db.yaml uses uv venv/uv run. This step should match them rather than introducing a third dependency-management style into the same repo.

Non-blocking / efficiency: this entire pngquant pass is thrown away by the very next pipeline step.

Both run_e2e_pipeline_test.sh (steps 2-3) and build-kotlin-docs.yaml (steps 2-3) unzip the same webHelpImages.zip twice. Here, insert_file pngquants and brotli-compresses each PNG into k/html/images/<name>; then insert_optimized_media.insert_optimized_file calls delete_content and re-inserts the re-optimized bytes at the same path.

With --webp it's not even an overwrite — the step-2 .png rows are orphaned and then swept up by delete_unreferenced_media.

On a doc set of ~1000 images that's ~1000 discarded pngquant subprocess invocations plus a full brotli pass, every run. Skipping media entirely in step 2 when step 3 is going to run should be a straight win on CI wall-clock.